Respect DI while creating target object #688
Conversation
dogukandemir
force-pushed
the
bugfix/respect-di-while-creating-target-object
branch
from
1d03f2e to
bc00585
Compare
| var isService = services.GetService<IServiceProviderIsService>()?.IsService(type) == true; | ||
|
|
||
| return isService | ||
| ? services.GetRequiredService(type) | ||
| : ActivatorUtilities.CreateInstance(services, type); |
There was a problem hiding this comment.
@stephentoub and I previously discussed whether or not we should try to resolve [McpServerToolType]'s directly from the DI container before using ActivatorUtilites. This is not something we do for ASP.NET Core MVC Controllers, but it is something we do for SignalR Hubs.
I can't find the conversation right now, but I remember recommending against resolving the tool/resource/prompt type from DI, because resolving SignalR Hubs from DI has been a source of bugs in SignalR in the past, and I hadn't heard people ask for MVC Controllers to get resolved directly from DI either.
I didn't consider the typed AddHttpClient scenario specifically, but MCP handlers are no different than MVC Controllers in this regard. Just as with Controllers, you have to inject the typed client into your MCP handler rather than make the MCP handler a typed client itself.
However, unlike MVC and SignalR, MCP handlers don't have currently have stateful properties like Controller.HttpContext or Hub.Clients.Caller. As it stands, it could be a perf optimization to register your handlers as singletons. The downside would be if we introduced a new handler base type in the future to make things like IMcpServer more discoverable inside handlers.
If we decide to go with this change, I think this could be simplified to the following since we always immediately resolve the service if it exists, and we're not trying to throw if the type is missing.
| var isService = services.GetService<IServiceProviderIsService>()?.IsService(type) == true; | |
| return isService | |
| ? services.GetRequiredService(type) | |
| : ActivatorUtilities.CreateInstance(services, type); | |
| return services.GetService(type) ?? ActivatorUtilities.CreateInstance(services, type); |
GitHub appears to be messing with the diff, but the above should replace lines 793-797.
There was a problem hiding this comment.
I almost forgot we merged #570 which allows you to inject the current IMcpServer into constructors and not just the handler method. If someone were to inject an IMcpServer into their MCP handler type and register it as a singleton, they would run into problems.
|
Thanks for your contribution. After some further discussion with the team, we don't think it makes sense to resolve MCP handler types as services at this time. Registering an MCP handler type as a typed client does not align with the intended usage of typed clients which is to act as an HttpClient wrapper that gets injected into your app. So, in the case of the
https://learn.microsoft.com/dotnet/core/extensions/httpclient-factory#typed-clients If you don't want to define an extra type, you can use a named client instead. Also, given that we merged #570 which allows you to constructor inject |
2 participants
Prefer resolving target types from DI (if registered) and fall back to ActivatorUtilities, ensuring container registered services like AddHttpClient are honored.
Motivation and Context
Resolving issue #685. Target types were always created by ActivatorUtilities, bypassing DI registrations. This PR respects DI first and fall back to ActivatorUtilities.
How Has This Been Tested?
It's been tested in unit tests. In
McpServerBuilderExtensionsCreateTargetHelperTests, a tool is created and BaseAddress of registered HttpClient has been returned from the tool to verify typed HttpClient resolved correctly.Breaking Changes
There is no breaking behavioral change. This PR will extend CreateTarget capabilities by respecting services DI.
Types of changes
Checklist
Additional context
This PR closes #685